-
Notifications
You must be signed in to change notification settings - Fork 125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Themes] - Shopify Theme Console #4183
Conversation
Thanks for your contribution! Depending on what you are working on, you may want to request a review from a Shopify team:
|
Coverage report
Show files with reduced coverage 🔻
Test suite run success1804 tests passing in 822 suites. Report generated by 🧪jest coverage report action from ddbe9f5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @jamesmengo! Awesome stuff! 🔥 I've left only some minor comments :)
packages/theme/src/cli/utilities/theme-environment/storefront-session.ts
Show resolved
Hide resolved
packages/theme/src/cli/utilities/theme-environment/storefront-session.test.ts
Outdated
Show resolved
Hide resolved
3d579d1
to
4d9cf02
Compare
1d1b33a
to
5970c7e
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for this PR, @jamesmengo! Great stuff. I've left some comments, but the PR looks great, and I could not find any bugs 🚀
Besides the comments, I believe we could:
- rename
evaluater.ts
toevaluator.ts
- move
repl-theme-manager.ts
andpassword-prompt.ts
to thepackages/theme/src/cli/utilities/repl/
directory
What do you think?
Thanks again for this PR! :)
e7e3bc6
to
1bb97b8
Compare
1bb97b8
to
95c3031
Compare
521948f
to
6ce98d4
Compare
6ce98d4
to
6fd43a1
Compare
6fd43a1
to
ddbe9f5
Compare
Differences in type declarationsWe detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:
New type declarationsWe found no new type declarations in this PR Existing type declarationspackages/cli-kit/dist/public/node/http.d.ts@@ -7,7 +7,7 @@ export { FetchError, Request } from 'node-fetch';
* @returns A FormData object.
*/
export declare function formData(): FormData;
-export type Response = ReturnType<typeof nodeFetch>;
+export type Response = Awaited<ReturnType<typeof nodeFetch>>;
/**
* An interface that abstracts way node-fetch. When Node has built-in
* support for "fetch" in the standard library, we can drop the node-fetch
@@ -20,7 +20,7 @@ export type Response = ReturnType<typeof nodeFetch>;
* @param init - An object containing any custom settings that you want to apply to the request.
* @returns A promise that resolves with the response.
*/
-export declare function fetch(url: RequestInfo, init?: RequestInit): Response;
+export declare function fetch(url: RequestInfo, init?: RequestInit): Promise<Response>;
/**
* A fetch function to use with Shopify services. The function ensures the right
* TLS configuragion is used based on the environment in which the service is running
@@ -30,7 +30,7 @@ export declare function fetch(url: RequestInfo, init?: RequestInit): Response;
* @param init - An object containing any custom settings that you want to apply to the request.
* @returns A promise that resolves with the response.
*/
-export declare function shopifyFetch(url: RequestInfo, init?: RequestInit): Response;
+export declare function shopifyFetch(url: RequestInfo, init?: RequestInit): Promise<Response>;
/**
* Download a file from a URL to a local path.
*
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @jamesmengo! 🔥🚀
WHY are these changes introduced?
WHAT is this pull request doing?
Base branch for the
shopify theme console
TS implementationHow to test your changes?
pnpm shopify theme console --dev-preview
shopify theme console
Evaluate some expressions and look for parity differences
Intentional Differences:
null
rather thannil
in TS landMeasuring impact
How do we know this change was effective? Please choose one:
Checklist